Skip to content

[9.0] Replace escape_string with alternatives#8647

Open
sfayer wants to merge 2 commits into
DIRACGrid:rel-v9r0from
sfayer:sql_escstr_v9
Open

[9.0] Replace escape_string with alternatives#8647
sfayer wants to merge 2 commits into
DIRACGrid:rel-v9r0from
sfayer:sql_escstr_v9

Conversation

@sfayer

@sfayer sfayer commented Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Backport of #8645 & #8613.

BEGINRELEASENOTES
*Core
FIX: Replace escape_string with alternatives
*TransformationSystem
FIX: Parameterise SQL in TransformationSystem
ENDRELEASENOTES

@sfayer sfayer requested review from atsareg and fstagni as code owners June 29, 2026 18:24
@sfayer sfayer marked this pull request as draft June 29, 2026 18:54
@fstagni

fstagni commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

The integration (client) tests are all failing with:

LocalRepo/TestCode/DIRAC/tests/Integration/TransformationSystem/Test_Client_Transformation.py::TransformationClientChain::test_mix FAILED [100%]

=================================== FAILURES ===================================
______________________ TransformationClientChain.test_mix ______________________

self = <Integration.TransformationSystem.Test_Client_Transformation.TransformationClientChain testMethod=test_mix>

    def test_mix(self):
        res = self.transClient.addTransformation(
            "transName", "description", "longDescription", "MCSimulation", "Standard", "Manual", ""
        )
        self.assertTrue(res["OK"])
        transID = res["Value"]
    
        # parameters
        res = self.transClient.setTransformationParameter(transID, "aParamName", "aParamValue")
        self.assertTrue(res["OK"])
        res1 = self.transClient.getTransformationParameters(transID, "aParamName")
        self.assertTrue(res1["OK"])
        res2 = self.transClient.getTransformationParameters(transID, ["aParamName"])
        self.assertTrue(res2["OK"])
    
        # file status
        lfns = ["/aa/lfn.1.txt", "/aa/lfn.2.txt", "/aa/lfn.3.txt", "/aa/lfn.4.txt"]
        res = self.transClient.addFilesToTransformation(transID, lfns)
        self.assertTrue(res["OK"])
        res = self.transClient.getTransformationFiles({"TransformationID": transID, "LFN": lfns})
        self.assertTrue(res["OK"])
        for f in res["Value"]:
            self.assertEqual(f["Status"], TransformationFilesStatus.UNUSED)
            self.assertEqual(f["ErrorCount"], 0)
        res = self.transClient.setFileStatusForTransformation(transID, TransformationFilesStatus.ASSIGNED, lfns)
        self.assertTrue(res["OK"])
        res = self.transClient.getTransformationFiles({"TransformationID": transID, "LFN": lfns})
        for f in res["Value"]:
            self.assertEqual(f["Status"], TransformationFilesStatus.ASSIGNED)
            self.assertEqual(f["ErrorCount"], 0)
        res = self.transClient.getTransformationStats(transID)
        self.assertTrue(res["OK"])
        self.assertEqual(res["Value"], {TransformationFilesStatus.ASSIGNED: 4, "Total": 4})
        # Setting files MaxReset from Assigned should increment ErrorCount
        res = self.transClient.setFileStatusForTransformation(transID, "MaxReset", lfns)
        res = self.transClient.getTransformationFiles({"TransformationID": transID, "LFN": lfns})
        self.assertTrue(res["OK"])
        for f in res["Value"]:
            self.assertEqual(f["Status"], "MaxReset")
            self.assertEqual(f["ErrorCount"], 1)
        # Cycle through Unused -> Assigned This should not increment ErrorCount
        res = self.transClient.setFileStatusForTransformation(transID, TransformationFilesStatus.UNUSED, lfns)
        self.assertTrue(res["OK"])
        res = self.transClient.setFileStatusForTransformation(transID, TransformationFilesStatus.ASSIGNED, lfns)
        self.assertTrue(res["OK"])
        res = self.transClient.getTransformationFiles({"TransformationID": transID, "LFN": lfns})
        self.assertTrue(res["OK"])
        for f in res["Value"]:
            self.assertEqual(f["Status"], TransformationFilesStatus.ASSIGNED)
            self.assertEqual(f["ErrorCount"], 1)
        # Resetting files Unused from Assigned should increment ErrorCount
        res = self.transClient.setFileStatusForTransformation(transID, TransformationFilesStatus.UNUSED, lfns)
        self.assertTrue(res["OK"])
        res = self.transClient.getTransformationFiles({"TransformationID": transID, "LFN": lfns})
        self.assertTrue(res["OK"])
        for f in res["Value"]:
            self.assertEqual(f["Status"], TransformationFilesStatus.UNUSED)
            self.assertEqual(f["ErrorCount"], 2)
        res = self.transClient.setFileStatusForTransformation(transID, TransformationFilesStatus.ASSIGNED, lfns)
        self.assertTrue(res["OK"])
        # Set files Processed
        res = self.transClient.setFileStatusForTransformation(transID, TransformationFilesStatus.PROCESSED, lfns)
        self.assertTrue(res["OK"])
        res = self.transClient.getTransformationFiles({"TransformationID": transID, "LFN": lfns})
        self.assertTrue(res["OK"])
        for f in res["Value"]:
            self.assertEqual(f["Status"], TransformationFilesStatus.PROCESSED)
            self.assertEqual(f["ErrorCount"], 2)
        # Setting files Unused should have no effect
        res = self.transClient.setFileStatusForTransformation(transID, TransformationFilesStatus.UNUSED, lfns)
        self.assertTrue(res["OK"])
        res = self.transClient.getTransformationFiles({"TransformationID": transID, "LFN": lfns})
        self.assertTrue(res["OK"])
        for f in res["Value"]:
            self.assertEqual(f["Status"], TransformationFilesStatus.PROCESSED)
            self.assertEqual(f["ErrorCount"], 2)
        # Forcing files Unused should work
        res = self.transClient.setFileStatusForTransformation(
            transID, TransformationFilesStatus.UNUSED, lfns, force=True
        )
        self.assertTrue(res["OK"])
        res = self.transClient.getTransformationFiles({"TransformationID": transID, "LFN": lfns})
        self.assertTrue(res["OK"])
        for f in res["Value"]:
            self.assertEqual(f["Status"], TransformationFilesStatus.UNUSED)
            self.assertEqual(f["ErrorCount"], 2)
        # tasks
        res = self.transClient.addTaskForTransformation(transID, lfns)
        self.assertTrue(res["OK"])
        res = self.transClient.getTransformationTasks({"TransformationID": transID})
        self.assertTrue(res["OK"])
        taskIDs = []
        for task in res["Value"]:
            self.assertEqual(task["ExternalStatus"], "Created")
            taskIDs.append(task["TaskID"])
        self.transClient.setTaskStatus(transID, taskIDs, "Running")
        res = self.transClient.getTransformationTasks({"TransformationID": transID})
        for task in res["Value"]:
            self.assertEqual(task["ExternalStatus"], "Running")
        res = self.transClient.extendTransformation(transID, 5)
        self.assertTrue(res["OK"])
        res = self.transClient.getTransformationTasks({"TransformationID": transID})
        self.assertEqual(len(res["Value"]), 6)
        res = self.transClient.getTasksToSubmit(transID, 5)
        self.assertTrue(res["OK"])
    
        # logging
        res = self.transClient.setTransformationParameter(transID, "Status", "Active")
        self.assertTrue(res["OK"])
        res = self.transClient.getTransformationLogging(transID)
        self.assertTrue(res["OK"])
>       self.assertAlmostEqual(len(res["Value"]), 4)
E       AssertionError: 3 != 4 within 7 places (1 difference)

LocalRepo/TestCode/DIRAC/tests/Integration/TransformationSystem/Test_Client_Transformation.py:305: AssertionError

I see the above also for the PR targeting the rel-8r0 branch. Any quick idea?

@sfayer sfayer mentioned this pull request Jul 1, 2026
Open
@sfayer sfayer marked this pull request as ready for review July 1, 2026 08:28
@sfayer

sfayer commented Jul 1, 2026

Copy link
Copy Markdown
Member Author

Hi,

I've fixed up the failures now, full details are below (it now includes a backport), but they should all be ready for review. (It looks like the tests are failing for various DIRACX reasons now rather than problems with these PRs.)

Regards,
Simon

The one affecting the v8r0 branch was the MySQL connection tripping up over a unicode string; I saw that on the v9r0 branch too when I ran the tests locally. If you want to store unicode then you're meant to do some other specific set-up (pick charsets and encodings for the table). The defaults have changed so it's sometimes possible, but it's safer if we just drop the two unicode test case parameters for this function: They're more testing the exact MySQL version & config in the test-suite rather than how well the escaping is working! I've removed them from all 3 branches.

The TransformationSystem is error is due to a bug in the way the parameters to the database are escaped. This has been fixed by my proper parameterisation of the queries in 9.1. In the earlier branches it was relying on the escaping function using double quotes so that it could wrap them with single quotes: The new function uses single quotes so they get mixed up and generate invalid SQL. I've fixed this by backporting the proper parameterised query PR for the TransformationSystem. It's a clean cherry-pick on v9, I had to do more work on it for v8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fstagni fstagni Awaiting requested review from fstagni fstagni is a code owner
@atsareg atsareg Awaiting requested review from atsareg atsareg is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sfayer @fstagni